-
Notifications
You must be signed in to change notification settings - Fork 537
services/friendbot: Instrument friendbot to emit traces #5796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
services/friendbot: Instrument friendbot to emit traces #5796
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR instruments the friendbot service to emit OpenTelemetry traces that will be sent to Grafana Alloy and forwarded to Grafana Tempo for distributed tracing capabilities and performance monitoring.
- Added a new
utils/tracerpackage with OpenTelemetry configuration - Updated friendbot service to use context-aware tracing throughout the request lifecycle
- Added OpenTelemetry middleware to the HTTP router for automatic request tracing
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/tracer/tracer.go | New utility package for OpenTelemetry tracer initialization and configuration |
| services/friendbot/main.go | Updated to initialize tracing, add OpenTelemetry middleware, and use chi/v5 router |
| services/friendbot/internal/friendbot_handler.go | Added tracing instrumentation to HTTP request handlers |
| services/friendbot/internal/friendbot.go | Updated Pay method to accept context for tracing |
| services/friendbot/internal/minion.go | Added context parameter and span instrumentation to minion operations |
| services/friendbot/internal/minion_test.go | Updated test functions to pass context parameter |
| services/friendbot/internal/friendbot_test.go | Updated test functions to pass context parameter |
| services/friendbot/friendbot.cfg | Added otel_endpoint configuration |
| go.mod | Updated dependencies to include OpenTelemetry packages and chi/v5 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I am fixing the |
a0dded9 to
873e3fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments inline.
|
Before merging can you check that this will still function okay with quickstart? You can do that by building quickstart with the friendbot git ref pointing at your friendbot pr commit sha. |
16efad5 to
65ba471
Compare
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
46fc1de to
8eb2292
Compare
Signed-off-by: Satyam Zode <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm.
It does look like there are 10 alerts from the golintci run which should probably be addressed. It's erroring saying that the import is not specified in a depguard config. But there's no depguard config in this repo.
Depguard is enabled here:
Line 6 in 0e7db2c
| - depguard |
But I don't see any config for it. @tamirms @Shaptic does @satyamz need to add a depguard config with this import, or should we remove depguard if it isn't really in use?
Also, sharing this information in case it's tribal knowledge, but after merging, use Jenkins to escalate the release to prod, and then monitor the new versions to make sure it's working smoothly. You can try out friendbot hitting the URL directly or using the lab.stellar.org account create/funder to test it.
|
@leighmcculloch I don't think depgaurd is used anywhere so we can remove it. |
|
Team, Can we merge this if it looks good? |
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
CHANGELOG.mdwithin the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
services/friendbot: Instrument friendbot to emit traces. Traces will be sent to Grafana alloy which will send traces to Grafana tempo.
Trace looks like this in Grafana tempo:
Why
Known limitations
This PR updates several dependencies. We need land this carefully.
Issue:
https://github.com/stellar/ops/issues/4005